chore: enforce hybrid PQ key exchange in mTLS handshakes by mkmks · Pull Request #922 · zama-ai/kms

mkmks · 2026-10-05T10:34:31Z

Description

While rustls already supported PQ key change for a while, and we had enabled in mTLS between parties, we don't forbid explicitly classical key exchange which might lead to a downgrade by a malicious operator. This PR only permits hybrid PQ exchanges, preventing potential downgrades.

PR Checklist

Tick all that apply — by ticking I attest the item holds; justify any deviation in the description above.

titouantanguy

Changes do LGTM, but maybe others also have an opinion on what we want to support for the key exchange and cipher suite ?
cc @jot2re @dd23 @kc1212

titouantanguy · 2026-10-05T11:49:54Z

+                    let error = server_result.unwrap_err();
+                    assert!(
+                        matches!(
+                            error
+                                .get_ref()
+                                .and_then(|error| error.downcast_ref::<Error>()),
+                            Some(Error::PeerIncompatible(
+                                PeerIncompatible::NoKxGroupsInCommon
+                            ))
+                        ),
+                        "unexpected rejection for {:?}: {error}",
+                        group.name(),
+                    );
+                    assert!(client_result.is_err(), "client accepted {:?}", group.name());


nit, why an assert on the client's error but an unwrap on the server's one ?

titouantanguy · 2026-10-05T11:54:44Z


+/// Constructs mutually authenticated P2P TLS configurations with hybrid-only key exchange.
+///
+/// Both endpoints require TLS 1.3 and prefer [`kx_group::X25519MLKEM768`] over [`kx_group::SECP256R1MLKEM768`].


Any reason why we support both ? and in that order ?
I reckon compliance frameworks might enforce SECP256R1MLKEM768 as that's what NIST suggests ?

The reason was that rustls supports both, and whatever is the default came out first. Will pin the NIST recommendation, unless someone objects.

x25519 is generally consider a safer choice than NIST, in particular when it comes to NIST's *R1 curves as there has been a bit of fear of weaknesses in the randomness.
So I think it depends on whether we want to stick to NIST recommendations or we rather trust the DJ Bernstein and co gang.
I do not have a strong opinion but for a personal project I would pick X25519. But I think we at some point agreed to go with NIST recommendations whenever possible?

Ok, under further investigation X25519 is not FIPS approved. So we should probably go with secp256r1 as the default

titouantanguy · 2026-10-05T11:58:38Z

+where
+    R: ResolvesServerCert + ResolvesClientCert + 'static,
+{
+    let mut provider = default_provider();


This PR talks about handshake only, but I'm wondering do we want to force AES 256 as well ?

Good point, will restrict session ciphers too.

Note that this would be greater than the classical signature (128 bits) and PQ (192 bits). Still good but might be a bit overkill with current paramters

jot2re

Thanks a lot for taking care of this.
I left a few smaller comments, but I also passed the branch through Claude that came with some surprisingly good findings:

Nothing enforces the policy where the configs are used.

GrpcNetworkingManager::new and TlsIncoming in kms_impl.rs:758 accept any ClientConfig or ServerConfig. tests/security_mode.rs even passes a classical default config and expects success.
Any future caller can bypass the policy without anyone noticing.
Options: wrap the configs in a type (e.g. P2pTlsConfig) that only build_p2p_tls_config can create, or check crypto_provider().kx_groups when they are accepted.

Signature verification uses a different provider from the handshake.

AttestedVerifier::new (tls.rs:198) and WebPkiClientVerifier::builder / WebPkiServerVerifier::builder in add_context take their algorithms from the global CryptoProvider::get_default(), not the restricted P2P provider.
That's harmless today, since signatures aren't restricted. But the P2P policy now lives in two places, and the verifier depends on whatever global provider is installed.
Suggest a single p2p_crypto_provider() passed into both the builder and the verifier (builder_with_provider). That also gives one place to narrow signature schemes later, or to add ML-DSA certificates.
Note that the docs say certificate authentication is still classical, so the PQ protection covers confidentiality only, not authentication.

(minor) Sibling code doesn't follow the policy.

core/experiments/src/conf/party.rs:113 (client) and choreography/server.rs:61 (tonic ServerTlsConfig) still negotiate classical groups through the same GrpcNetworkingManager.
These are benchmark binaries, not production. But the docs say "P2P connections require hybrid", and benchmarks will measure smaller classical handshakes. Reusing the shared provider would fix both.

jot2re · 2026-10-06T08:43:58Z


 The meta store is in-memory only, so a reboot of the core forgets every known request and session ID. The KMS connector keeps the state of each request in its [persistent database](https://github.com/zama-ai/fhevm/tree/main/kms-connector/connector-db); its [kms-worker](https://github.com/zama-ai/fhevm/tree/main/kms-connector/crates/kms-worker) marks a request as sent and only polls for the result on a retry, so a request is not run more often than necessary across core reboots.

+P2P TLS requires TLS 1.3 with `X25519MLKEM768` or `secp256r1MLKEM768` key exchange, with X25519 first. The server and client use an explicit AWS-LC provider restricted to these hybrid groups. `threshold_networking::tls::build_p2p_tls_config` constructs both configurations, and `kms-server` supplies the attested verifier and certificate resolver. This policy applies to manual and automatic certificates. Classical-only and pure ML-KEM peers cannot connect. Certificate authentication uses classical signatures. Every peer must support at least one permitted hybrid group before deployment.


As far as I could read from the NIST documents MLKEM768 provides level 3, equivalent to 192 bit AES, whereas secp256r1 (and by extension) x25519 provides around 128 bits security. So I guess we either want to use P384 or MLKEM512 to have the levels consistent.
However, we already used MLKEM1024 with P384, so we have had a tendency of selecting PQ parameters higher than their classical counterparts.

jot2re · 2026-10-06T08:48:25Z


+/// Constructs mutually authenticated P2P TLS configurations with hybrid-only key exchange.
+///
+/// Both endpoints require TLS 1.3 and prefer [`kx_group::X25519MLKEM768`] over [`kx_group::SECP256R1MLKEM768`].


x25519 is generally consider a safer choice than NIST, in particular when it comes to NIST's *R1 curves as there has been a bit of fear of weaknesses in the randomness.
So I think it depends on whether we want to stick to NIST recommendations or we rather trust the DJ Bernstein and co gang.
I do not have a strong opinion but for a personal project I would pick X25519. But I think we at some point agreed to go with NIST recommendations whenever possible?

jot2re · 2026-10-06T09:17:26Z


+/// Constructs mutually authenticated P2P TLS configurations with hybrid-only key exchange.
+///
+/// Both endpoints require TLS 1.3 and prefer [`kx_group::X25519MLKEM768`] over [`kx_group::SECP256R1MLKEM768`].


Ok, under further investigation X25519 is not FIPS approved. So we should probably go with secp256r1 as the default

jot2re · 2026-10-06T09:18:39Z

+where
+    R: ResolvesServerCert + ResolvesClientCert + 'static,
+{
+    let mut provider = default_provider();


Note that this would be greater than the classical signature (128 bits) and PQ (192 bits). Still good but might be a bit overkill with current paramters

jot2re · 2026-10-06T09:20:30Z

+            }
+        }
+    }
+}


Claude pointed out that it would be a good idea to also add a test to validate the HelloRetryRequest path, to ensure that the next release can work gracefully with the 0.15 during a rolling upgrade. In theory it should work without issue, but would be good to have a test

jot2re · 2026-10-06T09:28:41Z

+        cfg: ClientConfig::builder_with_provider(provider).with_protocol_versions(&[&TLS13])?,
+    }
+    .with_custom_certificate_verifier(verifier)
+    .with_client_cert_resolver(cert_resolver);


Claude pointed out that session resuming skips the attested verifier, and hence leaves a potential attack vector. It should be easy to solve with client.resumption = Resumption::disabled(), server.session_storage = Arc::new(NoServerSessionStorage {}) and server.send_tls13_tickets = 0.

chore: enforce hybrid PQ key exchange in mTLS handshakes

3516d8f

mkmks requested a review from a team as a code owner October 5, 2026 10:34

cla-bot Bot added the cla-signed The CLA has been signed. label Oct 5, 2026

titouantanguy approved these changes Oct 5, 2026

View reviewed changes

jot2re requested changes Oct 6, 2026

View reviewed changes


		The meta store is in-memory only, so a reboot of the core forgets every known request and session ID. The KMS connector keeps the state of each request in its [persistent database](https://github.com/zama-ai/fhevm/tree/main/kms-connector/connector-db); its [kms-worker](https://github.com/zama-ai/fhevm/tree/main/kms-connector/crates/kms-worker) marks a request as sent and only polls for the result on a retry, so a request is not run more often than necessary across core reboots.

		P2P TLS requires TLS 1.3 with `X25519MLKEM768` or `secp256r1MLKEM768` key exchange, with X25519 first. The server and client use an explicit AWS-LC provider restricted to these hybrid groups. `threshold_networking::tls::build_p2p_tls_config` constructs both configurations, and `kms-server` supplies the attested verifier and certificate resolver. This policy applies to manual and automatic certificates. Classical-only and pure ML-KEM peers cannot connect. Certificate authentication uses classical signatures. Every peer must support at least one permitted hybrid group before deployment.

Conversation

mkmks commented Oct 5, 2026

Description

PR Checklist

Uh oh!

titouantanguy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titouantanguy Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jot2re left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

titouantanguy Oct 5, 2026 •

edited

Loading